Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace os.path for pathlib in NLU module #7118

Closed
wants to merge 28 commits into from

Conversation

RomuloSouza
Copy link
Contributor

Proposed changes:

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@sara-tagger
Copy link
Collaborator

Thanks for submitting a pull request 🚀 @JustinaPetr will take a look at it as soon as possible ✨

@silvasara silvasara requested review from a team and degiz and removed request for a team November 1, 2020 23:54
Copy link
Contributor

@degiz degiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for working on that! 🚀
Overall looks very good, just left several comments!

rasa/nlu/classifiers/diet_classifier.py Outdated Show resolved Hide resolved
rasa/nlu/classifiers/keyword_intent_classifier.py Outdated Show resolved Hide resolved
rasa/nlu/classifiers/mitie_intent_classifier.py Outdated Show resolved Hide resolved
rasa/nlu/classifiers/mitie_intent_classifier.py Outdated Show resolved Hide resolved
rasa/nlu/classifiers/sklearn_intent_classifier.py Outdated Show resolved Hide resolved
rasa/nlu/model.py Outdated Show resolved Hide resolved
rasa/nlu/model.py Outdated Show resolved Hide resolved
rasa/nlu/utils/__init__.py Outdated Show resolved Hide resolved
@degiz
Copy link
Contributor

degiz commented Nov 10, 2020

@silvasara Is the PR ready for the review again? If yes, please re-request the review 👍

@silvasara
Copy link
Contributor

silvasara commented Nov 10, 2020

@silvasara Is the PR ready for the review again? If yes, please re-request the review

Only the author of the PR can request the review again. @RomuloSouza , can you request it again, please?

@RomuloSouza RomuloSouza force-pushed the replace-os.path-pathlib branch from c4765e0 to 16a71c5 Compare November 10, 2020 14:34
@RomuloSouza RomuloSouza force-pushed the replace-os.path-pathlib branch from 16a71c5 to 8c7bebb Compare November 10, 2020 14:35
@RomuloSouza RomuloSouza requested a review from degiz November 10, 2020 14:45
Copy link
Contributor

@degiz degiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the comments 👍
Left a couple more, and we're good to go!

changelog/3153.improvement.md Outdated Show resolved Hide resolved
rasa/nlu/featurizers/sparse_featurizer/regex_featurizer.py Outdated Show resolved Hide resolved
@RomuloSouza RomuloSouza requested a review from degiz November 11, 2020 14:25
Copy link
Contributor

@degiz degiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks a lot 💯

@RomuloSouza
Copy link
Contributor Author

@degiz are you going to merge this? We'd like to continue the issue for the remaining modules, should we wait for these changes to be merged?

@degiz
Copy link
Contributor

degiz commented Nov 16, 2020

@RomuloSouza I've already approved this patch set, so feel free to update this branch with the latest master and merge it!

@RomuloSouza
Copy link
Contributor Author

@degiz I've updated with the latest master but I still can't merge it. Perhaps because the CI is breaking?

@degiz
Copy link
Contributor

degiz commented Nov 25, 2020

Perhaps because the CI is breaking?

Yes, that's the case. Please check the failing jobs and address the issues 👍

@RomuloSouza RomuloSouza force-pushed the replace-os.path-pathlib branch from c4ddf57 to b425e6c Compare December 6, 2020 20:32
@RomuloSouza
Copy link
Contributor Author

@degiz I fixed the issues in the CI and I still can't merge this PR

@degiz
Copy link
Contributor

degiz commented Dec 7, 2020

@RomuloSouza the branch is already behind the master, you need to update it again 😬

Base automatically changed from master to main January 22, 2021 11:15
@tmbo
Copy link
Member

tmbo commented Mar 10, 2021

@RomuloSouza it would be great if you could address the open issues so we don't loose out on this cool contribution - I'll close this as it is stale.

@tmbo tmbo closed this Mar 10, 2021
@ErickGiffoni ErickGiffoni deleted the replace-os.path-pathlib branch August 5, 2021 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants